[bug-fix] Fix reinstall-overwrites-kept-config: preserve config on plain reinstall after --keep-config by github-actions[bot] · Pull Request #3449 · github/spec-kit

github-actions · 2026-07-10T12:34:13Z

Bug fix — reinstall-overwrites-kept-config

Proposed fix for issue #3427, applying the remediation from the bug assessment.

Verdict: Valid · Severity: medium

Summary

When specify extension remove <ext> --keep-config is used, the extension is unregistered but its *-config.yml files survive in the extension directory. A subsequent plain specify extension add <ext> unconditionally deleted that directory before copying the fresh extension in, silently discarding the preserved config. The fix rescues those stranded config files into memory before the rmtree and writes them back after copytree, so user-customized values always win over the packaged defaults.

Changes

File	Change	Notes
`src/specify_cli/extensions/__init__.py`	modified	Before `rmtree(dest_dir)`, collect any `-config.yml` / `-config.local.yml` files from an unregistered `dest_dir` into memory; restore them after `copytree`
`tests/test_extensions.py`	added tests	`test_reinstall_after_keep_config_preserves_config` and `test_reinstall_after_keep_config_preserves_local_config`

Tests Added or Updated

tests/test_extensions.py::TestInstallFromDirectory::test_reinstall_after_keep_config_preserves_config — pins that a customized *-config.yml survives a remove --keep-config → plain reinstall cycle
tests/test_extensions.py::TestInstallFromDirectory::test_reinstall_after_keep_config_preserves_local_config — same for *-config.local.yml override files

Local Verification

No project test command could be exercised in this environment (no venv, no pytest available). The logic was verified by code inspection: the fix mirrors the existing backup/restore pattern used by the --force reinstall path, applying it to the previously-unhandled "unregistered but config-bearing directory" case.

Deviations from Assessment

None. The implementation follows the preferred remediation exactly as described.

Risks & Review Notes

Config files are read into memory; these files are typically < 1 KB so there is no memory concern.
The not self.registry.is_installed(manifest.id) guard ensures we only rescue configs when the extension is genuinely unregistered, avoiding picking up stale files from a different install.
The write-back happens after shutil.copytree, so packaged defaults are always superseded by the user's values — no risk of defaults silently winning.
install_from_zip() delegates to install_from_directory(), so it is covered without additional changes.
No API surface change; all changes are internal to install_from_directory().

Refs #3427 · cc @grafvonb

Fixes [Bug]: extension reinstall overwrites config preserved by remove --keep-config #3427

Generated by 🛠️ Fix Bug from Labeled Issue for issue #3427 · 449.5 AIC · ⌖ 15.2 AIC · ⊞ 34K · ◷

…all after --keep-config Apply the remediation from the bug assessment on issue #3427. Before the unconditional shutil.rmtree(dest_dir), scan dest_dir for any *-config.yml and *-config.local.yml files and hold their contents in memory. After shutil.copytree succeeds, write them back so user-customized values always win over the packaged defaults. This mirrors the existing backup/restore logic for the --force reinstall path but handles the case where remove --keep-config left config files behind in an unregistered extension directory. Refs #3427 Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Fixes extension config loss when reinstalling after remove --keep-config.

Changes:

Rescues and restores preserved extension configuration files.
Adds regression tests for standard and local configs.

Show a summary per file

File	Description
`src/specify_cli/extensions/__init__.py`	Adds preserved-config rescue and restoration.
`tests/test_extensions.py`	Adds reinstall config-preservation tests.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Files reviewed: 2/2 changed files
Comments generated: 3
Review effort level: Medium

Copilot · 2026-07-13T12:28:50Z

+        # Restore stranded configs rescued before the rmtree above.
+        for filename, content in stranded_configs.items():
+            (dest_dir / filename).write_bytes(content)


Fixed in 5507df0. The stranded-config restore loop is now placed immediately after shutil.copytree, before command, skill, and hook registration — so a failure in any of those steps leaves the preserved config in place.

Posted on behalf of @mnriem by GitHub Copilot (model: claude-sonnet-4.5, autonomous).

Copilot · 2026-07-13T12:28:57Z

+        stranded_configs: dict[str, bytes] = {}
+        if dest_dir.exists() and not self.registry.is_installed(manifest.id):
+            for cfg_file in (
+                list(dest_dir.glob("*-config.yml"))
+                + list(dest_dir.glob("*-config.local.yml"))
+            ):
+                if cfg_file.is_file() and not cfg_file.is_symlink():
+                    stranded_configs[cfg_file.name] = cfg_file.read_bytes()


Fixed in 5507df0 and bd55c59. stranded_configs now stores (bytes, mode) tuples (capturing cfg_file.stat().st_mode before the rmtree). After copytree the original mode is reapplied via target.chmod(mode & 0o660) — the mask strips setuid/setgid and world-write bits so only user/group read-write permissions are restored.

Posted on behalf of @mnriem by GitHub Copilot (model: claude-sonnet-4.5, autonomous).

…eserve file mode - Restore missing `test_install_force_without_existing` method declaration in tests/test_extensions.py so pytest collects it as a separate test. - Move stranded-config restoration to immediately after `copytree`, before command/skill/hook registration, so a failed registration step can't leave preserved configs permanently lost. - Store `(bytes, mode)` tuples instead of bare bytes when rescuing stranded configs, and reapply the original file mode after writing so permission bits (e.g. 0600 for credential files) are faithfully restored. Assisted-by: GitHub Copilot (model: claude-sonnet-4.5, autonomous)

Only preserve user/group read-write bits (mode & 0o660) to avoid restoring setuid, setgid, or world-writable permissions from a user-modified config file. Assisted-by: GitHub Copilot (model: claude-sonnet-4.5, autonomous)

Copilot

Review details

Files reviewed: 2/2 changed files
Comments generated: 2
Review effort level: Medium

Copilot · 2026-07-13T19:23:25Z

+        # Restore stranded configs rescued before the rmtree above.
+        for filename, (content, mode) in stranded_configs.items():


Fixed in fix: add copytree rollback path and strengthen regression test with packaged default config. The copytree call is now wrapped in a try/except BaseException block. If it raises (disk full, permission error, etc.), the rescued configs are written back — recreating dest_dir with mkdir(parents=True, exist_ok=True) if it was left absent or only partially created — before the exception is re-raised.

Posted on behalf of @mnriem by GitHub Copilot (model: claude-sonnet-4.6, autonomous).

Copilot · 2026-07-13T19:23:33Z

+        # Install once
+        manager.install_from_directory(
+            extension_dir, "0.1.0", register_commands=False
+        )


Fixed in the same commit. test_reinstall_after_keep_config_preserves_config now writes a default test-ext-config.yml (with model: default-model) into extension_dir before the first install. The user then customizes the installed copy to custom-model. After remove --keep-config + plain reinstall, the test asserts that custom-model is present and default-model is not — so a "restore only when absent" implementation would fail.

Posted on behalf of @mnriem by GitHub Copilot (model: claude-sonnet-4.6, autonomous).

…ackaged default config - Wrap shutil.copytree in a try/except BaseException so stranded configs rescued before rmtree are written back even if copytree fails mid-way (addresses review comment: configs were permanently lost on copy failure) - Add a packaged default config to extension_dir in the regression test so a naive 'restore only when absent' implementation would fail; assert the user's customized values beat the packaged defaults after reinstall Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous)

Copilot

Review details

Files reviewed: 2/2 changed files
Comments generated: 2
Review effort level: Medium

+                    target = dest_dir / filename
+                    target.write_bytes(content)
+                    target.chmod(mode & 0o660)


+            target = dest_dir / filename
+            target.write_bytes(content)
+            # Mask to only user/group read-write bits to avoid restoring
+            # setuid/setgid or world-writable permissions.
+            target.chmod(mode & 0o660)


github-actions Bot added automated bug-fix Trigger the bug-fix agentic workflow labels Jul 10, 2026

github-actions Bot mentioned this pull request Jul 10, 2026

[Bug]: extension reinstall overwrites config preserved by remove --keep-config #3427

Open

mnriem marked this pull request as ready for review July 10, 2026 16:30

mnriem self-requested a review as a code owner July 10, 2026 16:30

Copilot AI review requested due to automatic review settings July 10, 2026 16:30

Copilot started reviewing on behalf of mnriem July 10, 2026 16:30 View session

Copilot AI reviewed Jul 10, 2026

View reviewed changes

Copilot started work on behalf of mnriem July 13, 2026 12:24 View session

Copilot AI added 2 commits July 13, 2026 12:26

Copilot finished work on behalf of mnriem July 13, 2026 12:29

mnriem requested a review from Copilot July 13, 2026 14:22

Copilot started reviewing on behalf of mnriem July 13, 2026 14:23 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

Copilot started work on behalf of mnriem July 13, 2026 19:17 View session

Copilot finished work on behalf of mnriem July 13, 2026 19:23

mnriem requested a review from Copilot July 13, 2026 20:20

Copilot started reviewing on behalf of mnriem July 13, 2026 20:20 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[bug-fix] Fix reinstall-overwrites-kept-config: preserve config on plain reinstall after --keep-config#3449

[bug-fix] Fix reinstall-overwrites-kept-config: preserve config on plain reinstall after --keep-config#3449
github-actions[bot] wants to merge 4 commits into
mainfrom
fix/3427-reinstall-overwrites-kept-config-40c76b4f5dd346d6

github-actions Bot commented Jul 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Jul 13, 2026

Uh oh!

Copilot AI Jul 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jul 13, 2026

Uh oh!

Copilot AI Jul 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		# Restore stranded configs rescued before the rmtree above.
		for filename, (content, mode) in stranded_configs.items():

Uh oh!

Conversation

github-actions Bot commented Jul 10, 2026

Bug fix — reinstall-overwrites-kept-config

Summary

Changes

Tests Added or Updated

Local Verification

Deviations from Assessment

Risks & Review Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

Uh oh!

Copilot AI Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Copilot AI Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants